-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Conan (C/C++) conan.lock file support #1230
Conversation
Signed-off-by: Hiroaki KAWAI <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good -- just one small suggestion.
It also looks like there's a lint issue that needs a simple fix.
Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Christopher Phillips <[email protected]>
Language: pkg.CPP, | ||
Type: pkg.ConanPkg, | ||
MetadataType: pkg.ConanaMetadataType, | ||
Metadata: pkg.ConanMetadata{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capturing the name and version isn't really necessary. There is one or two other catalogers that do this for implementation-specific limitations, however, in this case it's redundant.
That being said, there is other information in a conan.lock
file that could be useful to capture (such as options
). It's probably worth taking a look at what other information could be provided and adding that to the metadata.
If we don't want to add any more elements, my recommendation now would be to not have a metadata struct at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagoodman I can add options
path
and context
as metadata fields and remove name
and version
here
Then I'll update the conanfile
to include nil
for the metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wagoodman it also looks like name
and version
are tied to the method for generating a PURL for a given conan
package. It might be useful to keep those fields so we can agnostically generate the PURL for either a lockfile
or conanfile
even if the information is redundant for the package struct:
https://github.com/anchore/syft/blob/main/syft/pkg/conan_metadata.go
Is there another space you would want this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, we're reusing the same metadata for logically a different things. I don't think we should do this. That is, the Metadata
is reserved for things that are specific from what this package was parsed from (I didn't see the reuse on the first review pass). We should probably be introducing a new metadata type to capture this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @wagoodman 's comment about not needing redundant name and version information, but 👍 from me
Signed-off-by: Christopher Phillips <[email protected]>
Talked offline with @wagoodman. @kzantow I enhanced the metadata struct with more fields from the lockfile. We'll keep |
One more small refactor incoming |
|
||
type conanLock struct { | ||
GraphLock struct { | ||
Nodes map[string]struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be map[string]pkg.ConanMetadata
instead of redefining these. That is, if the metadata is meant to raise up the raw packaging metadata, if we remove name/version then there would be no difference between this struct def and pkg.ConanMetadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong about this if options string
should really be transformed into options []string
(split by newline) or options map[string]string
if split by newline and =
. (same comment for similar fields)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update options to be the map suggestion and look to make the other fields updated to match this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those following along here are all possible fields for the conan.lock structure
https://github.com/conan-io/conan/blob/develop/conans/model/graph_lock.py#L101-L276
syft/pkg/conan_metadata.go
Outdated
@@ -8,6 +8,9 @@ import ( | |||
type ConanMetadata struct { | |||
Name string `mapstructure:"name" json:"name"` | |||
Version string `mapstructure:"version" json:"version"` | |||
Options string `mapstructure:"options" json:"options"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to regenerate the json schema due to change (might as well wait on this until we know what the final change is)
- add NameAndVersion method to parse ref from ConanMetadata - update parseConanfile to use new NameAndVersion method - update parseConanfile tests to use Ref vs Name/Version - update parseConanlock to use new NameAndVersion method - update packageurl to test new NameAndVersion method - add ConanMetadata test for PURL parsing Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Quick status on this one Currently separating out the metadata types per this comment: Updating the CLI tests and adding more fields to the new metadata object per this comment: |
- add conan_lock_metadata to use for conan lock Parser - revert changes to conan metadata - update inline struct for conan lock parser to match all fields - update tests to match new parsing of conan lock options Signed-off-by: Christopher Phillips <[email protected]>
Last change here will be to regenerate the schema, I'm still reviewing and trying to find examples of a conan lock file is all possible fields populated so we're sure the new metadata struct if constructed correctly |
- BREAKING: change ConanMetadata to use ref - ADDITION: add ConanaLockMetadata type Signed-off-by: Christopher Phillips <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
CLI passed on my local @kzantow so this 🤞 should be ready |
Signed-off-by: Christopher Phillips <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Signed-off-by: Christopher Phillips <[email protected]>
Thanks @hkwi for the contribution! We really appreciate the enhancement here and getting the ball rolling on this. We also appreciate your patience with the PR process. |
* main: (45 commits) feat: add RelationshipsBySourceOwnership to syft json output (#1248) fix: reset merged package into map; (#1258) refactor: Remove experimental Anchore Enterprise upload functionality (#1257) Update syft bootstrap tools to latest versions. (#1254) Update Stereoscope to d24c9d626b33fa720210b007a20767801827b532 (#1253) Update syft bootstrap tools to latest versions. (#1244) fix apkdb checksum representation (#1247) feat: add identifiable field to source object (#1243) feat: attest support for Singularity images (#1201) Update syft bootstrap tools to latest versions. (#1239) Update Stereoscope to 1b1b744a919964f38d14e1416fb3f25221b761ce (#1240) fix: Follow symlinks when searching for globs in all-layers scope (#1221) update requires to use list; remove field (#1234) Add Conan (C/C++) conan.lock file support (#1230) add sequence diagrams and flesh out TODO notes (#1233) Do not fail if unable to parse `.rpm` file (#1232) fix: support exclude patterns on Windows (#1228) Update syft bootstrap tools to latest versions. (#1225) Update Stereoscope to 56552770e555d764ea72b99d3c810326b27ead4a (#1224) Update syft bootstrap tools to latest versions. (#1223) ... Signed-off-by: Christopher Phillips <[email protected]>
Co-authored-by: Christopher Phillips <[email protected]>
Co-authored-by: Christopher Phillips <[email protected]> Signed-off-by: Christopher Phillips <[email protected]>
Co-authored-by: Christopher Phillips <[email protected]> Signed-off-by: Christopher Phillips <[email protected]>
Co-authored-by: Christopher Phillips <[email protected]>
extend #1083 and implement #1222